[feature] Add API endpoint for indoor map coordinates #828#976
Conversation
103a39c to
611f852
Compare
|
@dee077 Isn't this part of the GSoC project idea for the indoor map? |
|
Yes, this aligns with the GSoC project idea for the indoor map. However, at the time, it didn’t have the gsoc-idea label, so I wasn’t aware that it would be proposed as a GSoC idea for 2025. |
d3ca54d to
e1e9295
Compare
nemesifier
left a comment
There was a problem hiding this comment.
The output looks very similar to GeoJSON, but it's not.
Good progress @dee077 👏
Look at where we use GeoJSON (rest_framework_gis): https://github.com/search?q=repo%3Aopenwisp%2Fopenwisp-controller%20rest_framework_gis&type=code.
Let's paginate the results and ensure the frontend can load paginated results gradually without crashing if there's many floorplan points scattered on many floors.
c9ab3be to
f6bf373
Compare
bafe7c1 to
0b15b71
Compare
d77d518 to
69976d0
Compare
Updates
|
319676a to
eec111f
Compare
pandafy
left a comment
There was a problem hiding this comment.
@dee077 I found some minor issues on this PR which I missed in my previous review.
Since, all of these changes are minor, I will take care of them now so you can focus working on other tasks.
However, I ask you to review all the comments so we can avoid them in the future.
docs/user/rest-api.rst
Outdated
|
|
||
| .. note:: | ||
|
|
||
| this endpoint returns device coordinates from the first floor above |
There was a problem hiding this comment.
| this endpoint returns device coordinates from the first floor above | |
| This endpoint returns device coordinates from the first floor above |
| def get_floor_name(self, obj): | ||
| loc_name = obj.floorplan.location.name | ||
| floor_no = obj.floorplan.floor | ||
| return f"{loc_name} {ordinal(floor_no)} Floor" |
There was a problem hiding this comment.
I thought, I already respond to this question, my bad.
The django-loci model has the correct string representation for the object. Can you we that?
openwisp_controller/geo/api/views.py
Outdated
| def get_parent_queryset(self): | ||
| return DeviceLocation.objects.filter(location__pk=self.kwargs.get("pk")) |
There was a problem hiding this comment.
The get_parent_queryset() method is used to get the list of parent elements which has the organization field.
I believe returning the queryset of the Location object will be more appropriate here.
Right now, both parent and child models are same.
openwisp_controller/geo/api/views.py
Outdated
| filter_backends = [filters.DjangoFilterBackend] | ||
| filterset_class = IndoorCoordinatesFilter | ||
| pagination_class = IndoorCoodinatesViewPagination | ||
| organization_field = "content_object__organization" |
There was a problem hiding this comment.
While this works without any issues, for logical reasons I think `floorplan__organization" will be more appropriate here.
| self.assertEqual(r.status_code, 401) | ||
|
|
||
| @capture_any_output() | ||
| def test_indoor_coodinate_list(self): |
There was a problem hiding this comment.
There's a lack of check which verifies the complete response.
Ideally, we want the tests to fail when we make changes to the code. Right now, the tests are not exhaustive. E.g., if I remove the admin url from the response, then none of the tests would fail.
| r = self.client.get(reverse(url)) | ||
| self.assertEqual(r.status_code, 401) | ||
|
|
||
| @capture_any_output() |
There was a problem hiding this comment.
@dee077 why did you use capture_any_output for this test?
We generally use this to suppress noisy logging while running the test suite. In my testing this test did not generate any noise.
|
|
||
| class IndoorCoordinatesSerializer(serializers.ModelSerializer): | ||
| admin_edit_url = SerializerMethodField("get_admin_edit_url") | ||
| content_object_id = serializers.UUIDField( |
There was a problem hiding this comment.
I think, device_id would be more descriptive here instead of content_object_id.
docs/user/rest-api.rst
Outdated
| This endpoint returns device coordinates from the first floor above | ||
| ground (lowest non-negative floors) by default. If a location only has | ||
| negative floors (e.g. underground parking lot), then it will return | ||
| the closest floor to the ground (maximum negative floor). |
There was a problem hiding this comment.
| the closest floor to the ground (maximum negative floor). | |
| the closest floor to the ground (greatest negative floor). |
openwisp_controller/geo/utils.py
Outdated
| name="detail_location", | ||
| ), | ||
| path( | ||
| "api/v1/controller/location/<str:pk>/indoor-coordinates/", |
There was a problem hiding this comment.
| "api/v1/controller/location/<str:pk>/indoor-coordinates/", | |
| "api/v1/controller/location/<uuid:pk>/indoor-coordinates/", |
Please verify that trying to open an URL with an invalid UUID returns 404 (should be automatically handled by Django), if that's not the case please write a test for this and fix the bug (hopefully not necessary).
There was a problem hiding this comment.
PS: you have copied the URL definition structure from the URLs above, which also suffer from the same issue, I am fixing that in #1101.
Implemented API to return device coordinates for a given location ID. Fixes openwisp#828
d39ba0e to
8e81084
Compare

Implemented API to return device coordinates for a given location ID.
Fixes #828
Checklist
Reference to Existing Issue
Closes #828
Description of Changes
floorplan_coordinates_listto list coordinates